feat(core,egg): add V8 startup snapshot lifecycle hooks and APIs#5856
feat(core,egg): add V8 startup snapshot lifecycle hooks and APIs#5856
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5856 +/- ##
==========================================
+ Coverage 85.26% 85.33% +0.06%
==========================================
Files 666 667 +1
Lines 13238 13320 +82
Branches 1535 1544 +9
==========================================
+ Hits 11288 11366 +78
- Misses 1819 1824 +5
+ Partials 131 130 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
6c29b67
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://de03972f.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-snapshot-egg.egg-v3.pages.dev |
Deploying egg with
|
| Latest commit: |
6c29b67
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://49736283.egg-cci.pages.dev |
| Branch Preview URL: | https://feat-snapshot-egg.egg-cci.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for V8 startup snapshots in the Egg.js framework, enabling faster application startup by pre-loading metadata. It adds a snapshot configuration option to skip the initialization of servers, timers, and persistent connections during the build phase. Key additions include the buildSnapshot and restoreSnapshot APIs, a loadFinished promise to track full metadata loading, and serialize/deserialize callbacks to manage resources like loggers and messengers. The utility package is also updated to allow module interception via setSnapshotModuleLoader. Feedback focuses on ensuring that cleanup hooks are correctly registered even in snapshot mode, maintaining stable event handler references, and re-applying messenger safety guards after restoration.
packages/egg/src/lib/egg.ts
Outdated
| this.lifecycle.registerBeforeClose(async () => { | ||
| // close all cluster clients | ||
| for (const clusterClient of this.#clusterClients) { | ||
| await closeClusterClient(clusterClient); | ||
| } | ||
| this.#clusterClients = []; | ||
|
|
||
| // single process mode will close agent before app close | ||
| if (this.type === 'application' && this.options.mode === 'single') { | ||
| await this.agent?.close(); | ||
| } | ||
| // single process mode will close agent before app close | ||
| if (this.type === 'application' && this.options.mode === 'single') { | ||
| await this.agent?.close(); | ||
| } | ||
|
|
||
| ManifestStore.flushCompileCache(); | ||
| for (const logger of this.loggers.values()) { | ||
| logger.close(); | ||
| } | ||
| this.messenger.close(); | ||
| process.removeListener('unhandledRejection', this._unhandledRejectionHandler); | ||
| }); | ||
| ManifestStore.flushCompileCache(); | ||
| for (const logger of this.loggers.values()) { | ||
| logger.close(); | ||
| } | ||
| this.messenger.close(); | ||
| process.removeListener('unhandledRejection', this._unhandledRejectionHandler); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The registerBeforeClose hook is currently guarded by !this.options.snapshot. This means that if an application is started in snapshot mode (for building a snapshot), these critical cleanup tasks (closing cluster clients, loggers, messenger, and removing listeners) will never be registered. When the application is later restored from the snapshot and eventually closed, these resources will leak.
Since registerBeforeClose merely adds functions to an internal array in the lifecycle object, it is safe to call during snapshot building. The captured closures will be part of the snapshot and will execute correctly in the restored process when app.close() is called.
// register close function
this.lifecycle.registerBeforeClose(async () => {
// close all cluster clients
for (const clusterClient of this.#clusterClients) {
await closeClusterClient(clusterClient);
}
this.#clusterClients = [];
// single process mode will close agent before app close
if (this.type === 'application' && this.options.mode === 'single') {
await this.agent?.close();
}
ManifestStore.flushCompileCache();
for (const logger of this.loggers.values()) {
logger.close();
}
this.messenger.close();
process.removeListener('unhandledRejection', this._unhandledRejectionHandler);
});
packages/egg/src/lib/egg.ts
Outdated
| this._unhandledRejectionHandler = this._unhandledRejectionHandler.bind(this); | ||
| process.on('unhandledRejection', this._unhandledRejectionHandler); |
There was a problem hiding this comment.
Re-binding _unhandledRejectionHandler here creates a new function instance. If registerBeforeClose (or any other logic) uses process.removeListener with this._unhandledRejectionHandler, it might fail to remove the listener if the instance doesn't match.
It is better to bind the handler once in the constructor and then use that single instance for all on and removeListener calls, regardless of whether the app is in snapshot mode or not.
packages/egg/src/lib/agent.ts
Outdated
| v8.startupSnapshot.addDeserializeCallback(() => { | ||
| this.#startKeepAlive(); | ||
| }); |
There was a problem hiding this comment.
When the Agent is restored from a snapshot, the messenger is re-created. However, the _wrapMessenger() method (which provides safety warnings if messenger methods are called before the server has started) is not re-applied to the new messenger instance. This means the restored Agent will lack these important development-time guards.
v8.startupSnapshot.addDeserializeCallback(() => {
this.#startKeepAlive();
this._wrapMessenger();
});There was a problem hiding this comment.
Pull request overview
This PR extends Egg’s startup workflow to support V8 startup snapshots by introducing a “snapshot/metadata-only” loading mode, explicit snapshot build/restore entry points, and supporting utilities/tests to make module loading and lifecycle timing deterministic in snapshot contexts.
Changes:
- Add
snapshot: trueloading mode +loadFinishedpromise and snapshot serialize/deserialize callback registration onEggApplicationCore(plus snapshot-mode guards). - Introduce snapshot-oriented entry points:
startEggForSnapshot(),buildSnapshot(),restoreSnapshot(), and export them from the package. - Add snapshot module-loader interception support in
@eggjs/utilsimport helpers, plus new snapshot-focused tests and snapshot exports snapshots updates.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/utils/src/import.ts | Adds snapshot module loader interception and adjusts getRequire() for restricted snapshot require. |
| packages/utils/test/snapshot-import.test.ts | New tests for snapshot module loader behavior and getRequire() fallback. |
| packages/utils/test/snapshots/index.test.ts.snap | Updates utils public export snapshot to include setSnapshotModuleLoader. |
| packages/egg/src/lib/egg.ts | Adds snapshot option, loadFinished, snapshot guards, and registerSnapshotCallbacks() behavior. |
| packages/egg/src/lib/agent.ts | Adds snapshot-aware keep-alive behavior and agent-specific snapshot callbacks. |
| packages/egg/src/lib/application.ts | Skips bindEvents in snapshot mode. |
| packages/egg/src/lib/start.ts | Adds startEggForSnapshot() entry point for snapshot builds. |
| packages/egg/src/lib/snapshot.ts | New buildSnapshot() / restoreSnapshot() public APIs. |
| packages/egg/src/index.ts | Exports snapshot utilities from the package entry. |
| packages/egg/test/lib/snapshot.test.ts | New test suite covering snapshot APIs, callbacks, loadFinished, and guards. |
| packages/egg/test/snapshots/index.test.ts.snap | Updates egg public export snapshot to include new snapshot APIs. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| let _originalIsESM: boolean; | ||
|
|
||
| // We need to capture and restore isESM since setSnapshotModuleLoader mutates it. | ||
| // Use dynamic import to read the current value. | ||
| afterEach(async () => { | ||
| // Reset the snapshot loader by setting it to a no-op then clearing via | ||
| // module internals. Since there's no public "unset" API, we re-import | ||
| // and the module-level _snapshotModuleLoader remains set — but tests | ||
| // are isolated enough that this is fine. We'll use a different approach: | ||
| // just call setSnapshotModuleLoader with a passthrough that calls the | ||
| // real import, but that changes isESM. Instead, we accept that these | ||
| // tests run with the loader set and each test overrides it. | ||
| // Reset by overwriting with undefined via the setter trick: | ||
| // Actually we can't unset. Let's just re-import fresh for isolation. |
There was a problem hiding this comment.
setSnapshotModuleLoader() mutates module-level state (isESM and the internal loader) but this test suite never restores it: _originalIsESM is unused and the afterEach is effectively a no-op. Since the repo’s Vitest workspace uses isolate: false (shared module cache across files), leaving the snapshot loader installed can leak into unrelated tests and change importResolve() behavior. Please add real cleanup (restore the previous isESM and clear/unset the snapshot loader) or drop the unused variable/hook if cleanup isn’t possible.
| let _originalIsESM: boolean; | |
| // We need to capture and restore isESM since setSnapshotModuleLoader mutates it. | |
| // Use dynamic import to read the current value. | |
| afterEach(async () => { | |
| // Reset the snapshot loader by setting it to a no-op then clearing via | |
| // module internals. Since there's no public "unset" API, we re-import | |
| // and the module-level _snapshotModuleLoader remains set — but tests | |
| // are isolated enough that this is fine. We'll use a different approach: | |
| // just call setSnapshotModuleLoader with a passthrough that calls the | |
| // real import, but that changes isESM. Instead, we accept that these | |
| // tests run with the loader set and each test overrides it. | |
| // Reset by overwriting with undefined via the setter trick: | |
| // Actually we can't unset. Let's just re-import fresh for isolation. | |
| afterEach(() => { | |
| // Clear the snapshot module loader so module-level state (e.g. isESM) | |
| // does not leak into other test files when Vitest runs with isolate: false. | |
| setSnapshotModuleLoader(undefined as any); |
packages/utils/src/import.ts
Outdated
|
|
||
| /** | ||
| * Register a snapshot module loader that intercepts `importModule()` calls. | ||
| * | ||
| * When set, `importModule()` delegates to this loader instead of calling | ||
| * `import()` or `require()`. This is used by the V8 snapshot entry generator | ||
| * to provide pre-bundled modules — the bundler generates a static module map | ||
| * from the egg manifest and registers it via this API. | ||
| * | ||
| * Also sets `isESM = false` because the snapshot bundle is CJS and | ||
| * esbuild's `import.meta` polyfill causes incorrect ESM detection. | ||
| */ | ||
| export function setSnapshotModuleLoader(loader: SnapshotModuleLoader): void { | ||
| _snapshotModuleLoader = loader; | ||
| isESM = false; |
There was a problem hiding this comment.
setSnapshotModuleLoader() permanently overwrites the global isESM flag and provides no way to clear/unregister _snapshotModuleLoader. In practice this makes importModule()/importResolve() behavior order-dependent once the API is called (including across test files when Vitest shares module cache). Consider adding a supported “unset/restore” mechanism (e.g., setSnapshotModuleLoader(undefined), clearSnapshotModuleLoader(), or returning a restore function) and restoring the previous isESM value when the loader is removed.
| /** | |
| * Register a snapshot module loader that intercepts `importModule()` calls. | |
| * | |
| * When set, `importModule()` delegates to this loader instead of calling | |
| * `import()` or `require()`. This is used by the V8 snapshot entry generator | |
| * to provide pre-bundled modules — the bundler generates a static module map | |
| * from the egg manifest and registers it via this API. | |
| * | |
| * Also sets `isESM = false` because the snapshot bundle is CJS and | |
| * esbuild's `import.meta` polyfill causes incorrect ESM detection. | |
| */ | |
| export function setSnapshotModuleLoader(loader: SnapshotModuleLoader): void { | |
| _snapshotModuleLoader = loader; | |
| isESM = false; | |
| let _snapshotPreviousIsESM: boolean | undefined; | |
| /** | |
| * Register or clear a snapshot module loader that intercepts `importModule()` calls. | |
| * | |
| * When set, `importModule()` delegates to this loader instead of calling | |
| * `import()` or `require()`. This is used by the V8 snapshot entry generator | |
| * to provide pre-bundled modules — the bundler generates a static module map | |
| * from the egg manifest and registers it via this API. | |
| * | |
| * When a loader is registered, `isESM` is forced to `false` because the | |
| * snapshot bundle is CJS and esbuild's `import.meta` polyfill can cause | |
| * incorrect ESM detection. | |
| * | |
| * Passing `undefined` clears the loader and restores the previous `isESM` | |
| * value that was in effect before the first loader was registered. | |
| */ | |
| export function setSnapshotModuleLoader(loader?: SnapshotModuleLoader): void { | |
| if (loader) { | |
| // First-time registration: remember the current isESM value so we can restore it. | |
| if (!_snapshotModuleLoader) { | |
| _snapshotPreviousIsESM = isESM; | |
| } | |
| _snapshotModuleLoader = loader; | |
| isESM = false; | |
| } else { | |
| // Clear the loader and restore the previous isESM value if we recorded one. | |
| _snapshotModuleLoader = undefined; | |
| if (_snapshotPreviousIsESM !== undefined) { | |
| isESM = _snapshotPreviousIsESM; | |
| _snapshotPreviousIsESM = undefined; | |
| } | |
| } |
packages/egg/src/lib/egg.ts
Outdated
| if (!this.options.snapshot) { | ||
| // Listen the error that promise had not catch, then log it in common-error | ||
| this._unhandledRejectionHandler = this._unhandledRejectionHandler.bind(this); | ||
| process.on('unhandledRejection', this._unhandledRejectionHandler); | ||
|
|
||
| // register close function | ||
| this.lifecycle.registerBeforeClose(async () => { | ||
| // close all cluster clients | ||
| for (const clusterClient of this.#clusterClients) { | ||
| await closeClusterClient(clusterClient); | ||
| } | ||
| this.#clusterClients = []; | ||
| // register close function | ||
| this.lifecycle.registerBeforeClose(async () => { | ||
| // close all cluster clients | ||
| for (const clusterClient of this.#clusterClients) { | ||
| await closeClusterClient(clusterClient); | ||
| } | ||
| this.#clusterClients = []; | ||
|
|
||
| // single process mode will close agent before app close | ||
| if (this.type === 'application' && this.options.mode === 'single') { | ||
| await this.agent?.close(); | ||
| } | ||
| // single process mode will close agent before app close | ||
| if (this.type === 'application' && this.options.mode === 'single') { | ||
| await this.agent?.close(); | ||
| } | ||
|
|
||
| ManifestStore.flushCompileCache(); | ||
| for (const logger of this.loggers.values()) { | ||
| logger.close(); | ||
| } | ||
| this.messenger.close(); | ||
| process.removeListener('unhandledRejection', this._unhandledRejectionHandler); | ||
| }); | ||
| ManifestStore.flushCompileCache(); | ||
| for (const logger of this.loggers.values()) { | ||
| logger.close(); | ||
| } | ||
| this.messenger.close(); | ||
| process.removeListener('unhandledRejection', this._unhandledRejectionHandler); | ||
| }); | ||
| } |
There was a problem hiding this comment.
In snapshot mode, the app never registers the beforeClose hook that closes cluster clients, loggers, and the messenger (and removes the unhandledRejection listener). As a result, await app.close() will skip these cleanups whenever snapshot: true, which can leak file descriptors (loggers) and other resources in snapshot tooling/tests. Consider registering the beforeClose cleanup regardless of snapshot mode, and only gating the runtime-only startup pieces (e.g. startup timeout timer / dumpConfig-after-ready / initial unhandledRejection registration).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds V8 startup snapshot build/restore APIs and a snapshot-aware start path, introduces snapshot lifecycle hooks and triggers, makes agent keep-alive startup lazy, wires snapshot cleanup/restore in Egg lifecycle, exposes snapshot utilities from package entry, and adds tests for snapshot behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Build Process
participant App as Agent/Application
participant V8 as v8.StartupSnapshot
participant Runtime as Restored Process
Builder->>App: startEggForSnapshot(options)
App->>App: construct Agent & Application (mode:'single', snapshot:true)
App->>App: await agent.loadFinished & app.loadFinished
Builder->>App: triggerSnapshotWillSerialize()
App->>App: close messenger, clear loggers, remove handlers
Builder->>V8: setDeserializeMainFunction(deserializer)
V8->>Builder: create serialized snapshot
Runtime->>V8: process starts from snapshot
V8->>Runtime: invoke deserialize callback -> globalThis.__egg_snapshot_app assigned
Runtime->>App: triggerSnapshotDidDeserialize()
App->>App: recreate messenger, reattach handlers
App->>App: triggerConfigDidLoad() → didLoad → willReady → didReady
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/scripts/test/utils.ts (1)
15-20:⚠️ Potential issue | 🟡 MinorProcess filter may match unrelated processes in concurrent test runs.
The
--snapshot-blobcheck on line 19 matches any Node process system-wide containing that flag, without anchoring tobaseDir. In CI environments with parallel test suites, this could inadvertently kill processes from other test runs.Consider requiring both conditions for snapshot processes:
🛠️ Proposed fix to scope snapshot process matching
export async function cleanup(baseDir: string) { const processList = await findNodeProcess((x) => { const dir = isWindows ? baseDir.replace(/\\/g, '\\\\') : baseDir; const prefix = isWindows ? '\\"baseDir\\":\\"' : '"baseDir":"'; - return x.cmd.includes(`${prefix}${dir}`) || x.cmd.includes('--snapshot-blob'); + // Match processes by baseDir in JSON args, OR snapshot processes that reference this baseDir + const matchesBaseDir = x.cmd.includes(`${prefix}${dir}`) || x.cmd.includes(dir); + const isSnapshotProcess = x.cmd.includes('--snapshot-blob'); + return matchesBaseDir || (isSnapshotProcess && x.cmd.includes(dir)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/test/utils.ts` around lines 15 - 20, The process filter in cleanup (function cleanup, using findNodeProcess and variable baseDir/prefix/isWindows) currently matches any node process that contains '--snapshot-blob' system-wide; change the predicate so snapshot processes are only matched if they also reference the same baseDir: i.e., require both x.cmd.includes('--snapshot-blob') AND x.cmd.includes(`${prefix}${dir}`) when matching snapshot processes (keep the existing direct baseDir match unchanged), so the filter becomes a match for baseDir OR (snapshot-blob AND baseDir).tools/scripts/src/commands/stop.ts (1)
47-63:⚠️ Potential issue | 🟡 Minor
--titlefiltering won't work for snapshot processes.Line 48 documents that snapshot processes set title via
process.title, not in args. However, the filter on lines 56-58 checkscmd.includes(format(osRelated.titleTemplate, flags.title)), which only searches command-line arguments.The underlying issue is that
item.cmdcontains only command-line arguments fromps -wweo argsoutput (see helper.ts). Snapshot mode sets title via theEGG_SERVER_TITLEenvironment variable (start.ts line 286), which doesn't appear in the command string, whereas regular mode passes--title=...as a command argument (start.ts line 317).This means
eggctl stop --title=myappwill not match snapshot-started processes.Consider either:
- Also checking environment variables in process matching (would require platform-specific process listing changes)
- Documenting that
--titlefiltering doesn't apply to snapshot processes- Passing title in snapshot process args for consistency
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/src/commands/stop.ts` around lines 47 - 63, The current process filtering in findNodeProcesses uses item.cmd and checks cmd.includes(format(osRelated.titleTemplate, flags.title)), which misses snapshot-mode processes that set title via the EGG_SERVER_TITLE env var (see start.ts). To fix this, update the matching logic in findNodeProcesses to also consider the environment-based title for snapshot processes: if flags.title is provided, treat a process as matched when either the command args include format(osRelated.titleTemplate, flags.title) OR the process environment contains EGG_SERVER_TITLE equal to flags.title; ensure you reference item.env (or the source that provides env info) when available and fall back to the existing cmd-based check when env info is absent so behavior is unchanged on platforms without env access.
🧹 Nitpick comments (4)
packages/utils/test/snapshot-import.test.ts (2)
10-10: Unused variable declaration.
_originalIsESMis declared but never assigned or used. Consider removing it.🧹 Remove unused variable
describe('setSnapshotModuleLoader', () => { - let _originalIsESM: boolean; - // We need to capture and restore isESM since setSnapshotModuleLoader mutates it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/test/snapshot-import.test.ts` at line 10, The variable _originalIsESM is declared in the test but never assigned or referenced; remove the unused declaration to clean up the test file (remove the line declaring _originalIsESM in snapshot-import.test.ts), and run tests/lint to confirm no remaining references to _originalIsESM in functions or blocks associated with the snapshot import tests.
14-24: Consider adding a reset mechanism for test isolation.The
afterEachhook is effectively empty, and the comments acknowledge that there's no public API to unset_snapshotModuleLoader. While the current tests work because each overrides the loader, this creates implicit coupling between tests.Consider exporting a test-only reset function from the module, or accepting this as intentional for now and removing the misleading comments.
🧹 Simplify the no-op afterEach
- afterEach(async () => { - // Reset the snapshot loader by setting it to a no-op then clearing via - // module internals. Since there's no public "unset" API, we re-import - // and the module-level _snapshotModuleLoader remains set — but tests - // are isolated enough that this is fine. We'll use a different approach: - // just call setSnapshotModuleLoader with a passthrough that calls the - // real import, but that changes isESM. Instead, we accept that these - // tests run with the loader set and each test overrides it. - // Reset by overwriting with undefined via the setter trick: - // Actually we can't unset. Let's just re-import fresh for isolation. - }); + // Note: No reset needed — each test overrides the snapshot loader. + // Tests are ordered to avoid interference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/test/snapshot-import.test.ts` around lines 14 - 24, The afterEach comments note there's no public way to unset the module-level _snapshotModuleLoader; add a test-only reset function (e.g. export function resetSnapshotModuleLoader() in the module that clears or restores _snapshotModuleLoader to its default/no-op) and call it from this test file's afterEach to ensure isolation; update tests to import resetSnapshotModuleLoader and replace the long comment block with a single call to resetSnapshotModuleLoader (or, if you prefer not to add API, simply remove the misleading comments and leave afterEach empty).tools/scripts/src/commands/snapshot-build.ts (1)
409-409: Consider usingNumber()instead ofparseInt()for PORT.
parseInt(process.env.PORT)returnsNaNfor non-numeric strings (includingundefined), which the|| 7001fallback handles. However,parseInt("3000abc")would return3000, which might be unexpected.Using
Number()or adding explicit validation would be more robust, but this is a minor edge case.🔧 Optional: Stricter port parsing
- lines.push(' const port = parseInt(process.env.PORT) || 7001;'); + lines.push(' const port = Number(process.env.PORT) || 7001;');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/src/commands/snapshot-build.ts` at line 409, The current port parsing uses parseInt(process.env.PORT) which accepts strings like "3000abc"; update the port parsing for snapshot-build.ts so port is parsed with Number(process.env.PORT) and validated (e.g., check Number.isInteger(port) and that port is within 1–65535) before falling back to 7001; locate the expression setting the port (const port = parseInt(process.env.PORT) || 7001;) and replace it with numeric conversion plus explicit validation and fallback to ensure only a valid integer port is used.packages/utils/src/import.ts (1)
399-411: Minor redundancy in the guard condition.On line 402, the condition
obj.default?.__esModule === true && obj.default && 'default' in obj.defaulthas a redundant&& obj.default— ifobj.default?.__esModule === trueevaluates totrue, thenobj.defaultmust already be truthy.However, this mirrors the fix applied to line 426 and maintains consistency with the existing pattern, so this is a minor nit.
🔧 Optional: Simplify the guard
- if (obj && typeof obj === 'object' && obj.default?.__esModule === true && obj.default && 'default' in obj.default) { + if (obj && typeof obj === 'object' && obj.default?.__esModule === true && 'default' in obj.default) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/src/import.ts` around lines 399 - 411, The guard inside the _snapshotModuleLoader branch redundantly checks "&& obj.default" after "obj.default?.__esModule === true"; update the condition in the block where you set obj = obj.default to remove the redundant && obj.default so it mirrors the later pattern (i.e., rely on obj.default?.__esModule === true and "'default' in obj.default" as the checks) and keep the subsequent importDefaultOnly branch unchanged; edit the conditional surrounding obj assignment in the _snapshotModuleLoader handling (referencing _snapshotModuleLoader, moduleFilePath, obj, and options?.importDefaultOnly) to simplify the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/scripts/scripts/start-single.mjs`:
- Around line 12-16: The ESM loader only checks framework.start ??
framework.startEgg and misses cases where the framework exposes start/startEgg
on its default export; update the resolution in the start logic (after
importModule(options.framework)) to fall back to framework.default?.start ??
framework.default?.startEgg when typeof startEgg !== 'function', ensuring
startEgg resolves the same way as the CJS startEgg resolution.
---
Outside diff comments:
In `@tools/scripts/src/commands/stop.ts`:
- Around line 47-63: The current process filtering in findNodeProcesses uses
item.cmd and checks cmd.includes(format(osRelated.titleTemplate, flags.title)),
which misses snapshot-mode processes that set title via the EGG_SERVER_TITLE env
var (see start.ts). To fix this, update the matching logic in findNodeProcesses
to also consider the environment-based title for snapshot processes: if
flags.title is provided, treat a process as matched when either the command args
include format(osRelated.titleTemplate, flags.title) OR the process environment
contains EGG_SERVER_TITLE equal to flags.title; ensure you reference item.env
(or the source that provides env info) when available and fall back to the
existing cmd-based check when env info is absent so behavior is unchanged on
platforms without env access.
In `@tools/scripts/test/utils.ts`:
- Around line 15-20: The process filter in cleanup (function cleanup, using
findNodeProcess and variable baseDir/prefix/isWindows) currently matches any
node process that contains '--snapshot-blob' system-wide; change the predicate
so snapshot processes are only matched if they also reference the same baseDir:
i.e., require both x.cmd.includes('--snapshot-blob') AND
x.cmd.includes(`${prefix}${dir}`) when matching snapshot processes (keep the
existing direct baseDir match unchanged), so the filter becomes a match for
baseDir OR (snapshot-blob AND baseDir).
---
Nitpick comments:
In `@packages/utils/src/import.ts`:
- Around line 399-411: The guard inside the _snapshotModuleLoader branch
redundantly checks "&& obj.default" after "obj.default?.__esModule === true";
update the condition in the block where you set obj = obj.default to remove the
redundant && obj.default so it mirrors the later pattern (i.e., rely on
obj.default?.__esModule === true and "'default' in obj.default" as the checks)
and keep the subsequent importDefaultOnly branch unchanged; edit the conditional
surrounding obj assignment in the _snapshotModuleLoader handling (referencing
_snapshotModuleLoader, moduleFilePath, obj, and options?.importDefaultOnly) to
simplify the guard.
In `@packages/utils/test/snapshot-import.test.ts`:
- Line 10: The variable _originalIsESM is declared in the test but never
assigned or referenced; remove the unused declaration to clean up the test file
(remove the line declaring _originalIsESM in snapshot-import.test.ts), and run
tests/lint to confirm no remaining references to _originalIsESM in functions or
blocks associated with the snapshot import tests.
- Around line 14-24: The afterEach comments note there's no public way to unset
the module-level _snapshotModuleLoader; add a test-only reset function (e.g.
export function resetSnapshotModuleLoader() in the module that clears or
restores _snapshotModuleLoader to its default/no-op) and call it from this test
file's afterEach to ensure isolation; update tests to import
resetSnapshotModuleLoader and replace the long comment block with a single call
to resetSnapshotModuleLoader (or, if you prefer not to add API, simply remove
the misleading comments and leave afterEach empty).
In `@tools/scripts/src/commands/snapshot-build.ts`:
- Line 409: The current port parsing uses parseInt(process.env.PORT) which
accepts strings like "3000abc"; update the port parsing for snapshot-build.ts so
port is parsed with Number(process.env.PORT) and validated (e.g., check
Number.isInteger(port) and that port is within 1–65535) before falling back to
7001; locate the expression setting the port (const port =
parseInt(process.env.PORT) || 7001;) and replace it with numeric conversion plus
explicit validation and fallback to ensure only a valid integer port is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e3f6d8e-e144-4450-85f6-3039372088ad
⛔ Files ignored due to path filters (3)
packages/egg/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snappackages/utils/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
packages/egg/src/index.tspackages/egg/src/lib/agent.tspackages/egg/src/lib/application.tspackages/egg/src/lib/egg.tspackages/egg/src/lib/snapshot.tspackages/egg/src/lib/start.tspackages/egg/test/lib/snapshot.test.tspackages/utils/src/import.tspackages/utils/test/snapshot-import.test.tstools/scripts/package.jsontools/scripts/scripts/start-single.cjstools/scripts/scripts/start-single.mjstools/scripts/src/commands/snapshot-build.tstools/scripts/src/commands/start.tstools/scripts/src/commands/stop.tstools/scripts/src/helper.tstools/scripts/src/index.tstools/scripts/test/fixtures/snapshot-app/.gitignoretools/scripts/test/fixtures/snapshot-app/app/router.jstools/scripts/test/fixtures/snapshot-app/config/config.default.jstools/scripts/test/fixtures/snapshot-app/package.jsontools/scripts/test/snapshot.test.tstools/scripts/test/utils.tstools/scripts/tsdown.config.ts
| const framework = await importModule(options.framework); | ||
| const startEgg = framework.start ?? framework.startEgg; | ||
| if (typeof startEgg !== 'function') { | ||
| throw new Error(`Cannot find start/startEgg function from framework: ${options.framework}`); | ||
| } |
There was a problem hiding this comment.
Inconsistent export resolution compared to CJS version.
The ESM version only checks framework.start ?? framework.startEgg, but the CJS version (start-single.cjs lines 12-16) also handles default exports:
// CJS version handles:
let startEgg = exports.start ?? exports.startEgg;
if (typeof startEgg !== 'function') {
startEgg = exports.default?.start ?? exports.default?.startEgg;
}This inconsistency could cause the ESM version to fail for frameworks that only expose start/startEgg via default export.
🔧 Proposed fix to align with CJS behavior
const framework = await importModule(options.framework);
-const startEgg = framework.start ?? framework.startEgg;
+let startEgg = framework.start ?? framework.startEgg;
+if (typeof startEgg !== 'function') {
+ startEgg = framework.default?.start ?? framework.default?.startEgg;
+}
if (typeof startEgg !== 'function') {
throw new Error(`Cannot find start/startEgg function from framework: ${options.framework}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/scripts/scripts/start-single.mjs` around lines 12 - 16, The ESM loader
only checks framework.start ?? framework.startEgg and misses cases where the
framework exposes start/startEgg on its default export; update the resolution in
the start logic (after importModule(options.framework)) to fall back to
framework.default?.start ?? framework.default?.startEgg when typeof startEgg !==
'function', ensuring startEgg resolves the same way as the CJS startEgg
resolution.
killagu
force-pushed
the
feat/snapshot-egg
branch
from
ab375cd to
15150ca
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/egg/src/lib/egg.ts (1)
616-627: Serialize callback removes listener that may not have been registered.In snapshot mode, the
unhandledRejectionlistener is never registered (guarded at line 242), but the serialize callback at line 626 attempts to remove it. Whileprocess.removeListeneris a no-op for non-existent listeners, this could be confusing for maintainers.Consider adding a comment or guard:
📝 Suggested clarification
this.messenger.close(); + // Safe no-op if listener was never registered (snapshot mode skips registration) process.removeListener('unhandledRejection', this._unhandledRejectionHandler);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/src/lib/egg.ts` around lines 616 - 627, The serialize callback registered in registerSnapshotCallbacks (via v8.startupSnapshot.addSerializeCallback) calls process.removeListener('unhandledRejection', this._unhandledRejectionHandler) even when that listener may never have been registered; update the callback to either check whether the listener was added (track a boolean flag set where the listener is registered, e.g., alongside the registration code) or add an explanatory comment before the removeListener call to clarify it's intentionally tolerant of non-registered handlers; reference the existing symbols registerSnapshotCallbacks, v8.startupSnapshot.addSerializeCallback, and this._unhandledRejectionHandler when making the change so reviewers can find the relevant code.packages/egg/src/lib/start.ts (1)
98-118: Consider extracting framework resolution to a shared helper.The framework resolution logic (reading
package.json, importing framework module, deriving Agent/Application classes) is duplicated betweenstartEgg()(lines 44-60) andstartEggForSnapshot()(lines 102-118). This could be extracted into a private helper function to reduce duplication.♻️ Example refactor
+interface FrameworkClasses { + Agent: typeof Agent; + Application: typeof Application; +} + +async function resolveFrameworkClasses(baseDir: string, framework?: string): Promise<FrameworkClasses> { + let resolvedFramework = framework; + if (!resolvedFramework) { + try { + const pkg = await readJSON(path.join(baseDir, 'package.json')); + resolvedFramework = pkg.egg.framework; + } catch { + // ignore + } + } + let AgentClass = Agent; + let ApplicationClass = Application; + if (resolvedFramework) { + const frameworkModule = await importModule(resolvedFramework, { + paths: [baseDir], + }); + AgentClass = frameworkModule.Agent; + ApplicationClass = frameworkModule.Application; + } + return { Agent: AgentClass, Application: ApplicationClass }; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/src/lib/start.ts` around lines 98 - 118, Extract the duplicated framework resolution in startEggForSnapshot and startEgg into a single private helper (e.g., resolveFrameworkClasses) that accepts SnapshotEggOptions/baseDir and optional framework name and returns { AgentClass, ApplicationClass } (defaulting to Agent and Application). Implement the helper to: read package.json via readJSON when framework not provided, resolve framework via importModule(paths: [baseDir]) when found, and fall back to the built-in Agent/Application; then replace the duplicated blocks in startEggForSnapshot and startEgg to call this helper and assign AgentClass/ApplicationClass from its result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/egg/src/lib/egg.ts`:
- Around line 616-627: The serialize callback registered in
registerSnapshotCallbacks (via v8.startupSnapshot.addSerializeCallback) calls
process.removeListener('unhandledRejection', this._unhandledRejectionHandler)
even when that listener may never have been registered; update the callback to
either check whether the listener was added (track a boolean flag set where the
listener is registered, e.g., alongside the registration code) or add an
explanatory comment before the removeListener call to clarify it's intentionally
tolerant of non-registered handlers; reference the existing symbols
registerSnapshotCallbacks, v8.startupSnapshot.addSerializeCallback, and
this._unhandledRejectionHandler when making the change so reviewers can find the
relevant code.
In `@packages/egg/src/lib/start.ts`:
- Around line 98-118: Extract the duplicated framework resolution in
startEggForSnapshot and startEgg into a single private helper (e.g.,
resolveFrameworkClasses) that accepts SnapshotEggOptions/baseDir and optional
framework name and returns { AgentClass, ApplicationClass } (defaulting to Agent
and Application). Implement the helper to: read package.json via readJSON when
framework not provided, resolve framework via importModule(paths: [baseDir])
when found, and fall back to the built-in Agent/Application; then replace the
duplicated blocks in startEggForSnapshot and startEgg to call this helper and
assign AgentClass/ApplicationClass from its result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49d3ebcd-c068-47c9-a96b-c260bbf97973
⛔ Files ignored due to path filters (1)
packages/egg/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
packages/egg/src/index.tspackages/egg/src/lib/agent.tspackages/egg/src/lib/application.tspackages/egg/src/lib/egg.tspackages/egg/src/lib/snapshot.tspackages/egg/src/lib/start.tspackages/egg/test/lib/snapshot.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/egg/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/egg/src/lib/application.ts
- packages/egg/src/lib/snapshot.ts
- packages/egg/src/lib/agent.ts
- packages/egg/test/lib/snapshot.test.ts
killagu
force-pushed
the
feat/snapshot-egg
branch
from
15150ca to
e16fa39
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/lifecycle.ts`:
- Around line 404-470: The triggerSnapshotDidDeserialize entrypoint must only
run in snapshot mode and must be idempotent: add a guard at the top of
triggerSnapshotDidDeserialize that (1) checks an options snapshot flag (e.g.
this.options.snapshot or this.app.options.snapshot) and returns early (or
throws) if not in snapshot mode, and (2) checks/sets a private "already
restored" flag (e.g. this.#snapshotDeserialized or similar) so a second call is
a no-op; ensure this flag is set before resetting this.#readyObject, calling
this.#initReady(), this.ready(...)/this.triggerDidReady, and
this.triggerConfigDidLoad to avoid duplicate boot side-effects and beforeClose
registrations.
- Around line 404-421: The snapshot hook error handlers currently only emit
errors and continue, which lets snapshot operations proceed in an inconsistent
state; update the catch blocks in triggerSnapshotWillSerialize and
triggerSnapshotDidDeserialize to rethrow the caught error (after emitting it) so
the Promise rejects and callers can abort; specifically, inside the try/catch
around await utils.callFn(boot.snapshotWillSerialize.bind(boot)) and the
analogous call to boot.snapshotDidDeserialize, call this.emit('error', err) and
then throw err (ensuring timing.end still runs as needed or wrap timing.end in
finally if required) so the first hook failure propagates to the caller.
In `@packages/egg/src/lib/egg.ts`:
- Around line 166-169: The loadFinished promise currently only delays callers
but does not prevent load() from registering ready-side effects; modify load()
so that it does not register the ready callbacks that perform startup side
effects (the ready(() => { dumpConfig(); dumpTiming(); dumpManifest(); ... })
registrations) and does not call `#setupTimeoutTimer`() when running in
snapshot/build mode; detect snapshot mode via the existing snapshot flag/option
used by the lifecycle (the same condition that marks ready after configWillLoad)
and skip registering those callbacks and timers so snapshot builds won't execute
dumpConfig/dumpTiming/dumpManifest or `#setupTimeoutTimer`() in the builder
process.
In `@packages/egg/test/snapshot.test.ts`:
- Around line 180-190: The test must assert loadFinished is still pending when
ready() resolves: capture the settled state of app.loadFinished before awaiting
ready() (use a small isPending/isSettled helper or Promise.race trick against a
resolved sentinel to detect whether app.loadFinished has settled), assert it is
pending, then await app.ready(), then assert app.loadFinished remains pending
(i.e. not settled) after ready() resolves, and finally await app.loadFinished to
complete the test; reference Application, app.ready(), and app.loadFinished when
locating where to add the checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4fae92d-5af4-4c53-b369-c59af8c75594
📒 Files selected for processing (9)
packages/core/src/egg.tspackages/core/src/lifecycle.tspackages/core/test/snapshot.test.tspackages/egg/src/index.tspackages/egg/src/lib/agent.tspackages/egg/src/lib/egg.tspackages/egg/src/lib/snapshot.tspackages/egg/src/lib/start.tspackages/egg/test/snapshot.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/egg/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/egg/src/lib/snapshot.ts
- packages/egg/src/lib/agent.ts
- packages/egg/src/lib/start.ts
| async triggerSnapshotWillSerialize(): Promise<void> { | ||
| debug('trigger snapshotWillSerialize start'); | ||
| const boots = [...this.#boots].reverse(); | ||
| for (const boot of boots) { | ||
| if (typeof boot.snapshotWillSerialize !== 'function') { | ||
| continue; | ||
| } | ||
| const fullPath = boot.fullPath ?? 'unknown'; | ||
| debug('trigger snapshotWillSerialize at %o', fullPath); | ||
| const timingKey = `Snapshot Will Serialize in ${utils.getResolvedFilename(fullPath, this.app.baseDir)}`; | ||
| this.timing.start(timingKey); | ||
| try { | ||
| await utils.callFn(boot.snapshotWillSerialize.bind(boot)); | ||
| } catch (err) { | ||
| debug('trigger snapshotWillSerialize error at %o, error: %s', fullPath, err); | ||
| this.emit('error', err); | ||
| } | ||
| this.timing.end(timingKey); | ||
| } | ||
| debug('trigger snapshotWillSerialize end'); | ||
| } | ||
|
|
||
| /** | ||
| * Trigger snapshotDidDeserialize on all boots in FORWARD order. | ||
| * Called by the restore entry after V8 deserializes the heap. | ||
| * After all hooks complete, resets the ready state and resumes the normal | ||
| * lifecycle from configDidLoad. The returned promise resolves when the | ||
| * full lifecycle (configDidLoad → didLoad → willReady) has completed. | ||
| */ | ||
| async triggerSnapshotDidDeserialize(): Promise<void> { | ||
| debug('trigger snapshotDidDeserialize start'); | ||
| for (const boot of this.#boots) { | ||
| if (typeof boot.snapshotDidDeserialize !== 'function') { | ||
| continue; | ||
| } | ||
| const fullPath = boot.fullPath ?? 'unknown'; | ||
| debug('trigger snapshotDidDeserialize at %o', fullPath); | ||
| const timingKey = `Snapshot Did Deserialize in ${utils.getResolvedFilename(fullPath, this.app.baseDir)}`; | ||
| this.timing.start(timingKey); | ||
| try { | ||
| await utils.callFn(boot.snapshotDidDeserialize.bind(boot)); | ||
| } catch (err) { | ||
| debug('trigger snapshotDidDeserialize error at %o, error: %s', fullPath, err); | ||
| this.emit('error', err); | ||
| } | ||
| this.timing.end(timingKey); | ||
| } | ||
| debug('trigger snapshotDidDeserialize end'); | ||
|
|
||
| // Reset ready state for the resumed lifecycle. | ||
| // In snapshot mode, ready(true) was called during triggerConfigWillLoad, | ||
| // resolving the ready promise early. We need fresh ready objects so the | ||
| // resumed lifecycle (didLoad → willReady → didReady) can track properly. | ||
| // Note: keep options.snapshot = true so the constructor's stale ready | ||
| // callback (which may fire asynchronously) correctly skips triggerDidReady. | ||
| this.#readyObject = new ReadyObject(); | ||
| this.#initReady(); | ||
| this.ready((err) => { | ||
| void this.triggerDidReady(err); | ||
| debug('app ready after snapshot deserialize'); | ||
| }); | ||
|
|
||
| // Resume the normal lifecycle from configDidLoad | ||
| this.triggerConfigDidLoad(); | ||
|
|
||
| // Wait for the full resumed lifecycle to complete | ||
| await this.ready(); |
There was a problem hiding this comment.
Guard these entrypoints so they only run in the snapshot flow.
triggerSnapshotDidDeserialize() currently resets #readyObject and reruns configDidLoad → didLoad → willReady with no options.snapshot or phase check. An accidental call on a normal app—or a second deserialize call—will duplicate boot side effects and beforeClose registrations. For example, the agent keepalive boot is driven from configDidLoad(), so a second pass can recreate runtime state unexpectedly. An early snapshot/one-shot assert would keep this public API from corrupting normal app state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/lifecycle.ts` around lines 404 - 470, The
triggerSnapshotDidDeserialize entrypoint must only run in snapshot mode and must
be idempotent: add a guard at the top of triggerSnapshotDidDeserialize that (1)
checks an options snapshot flag (e.g. this.options.snapshot or
this.app.options.snapshot) and returns early (or throws) if not in snapshot
mode, and (2) checks/sets a private "already restored" flag (e.g.
this.#snapshotDeserialized or similar) so a second call is a no-op; ensure this
flag is set before resetting this.#readyObject, calling this.#initReady(),
this.ready(...)/this.triggerDidReady, and this.triggerConfigDidLoad to avoid
duplicate boot side-effects and beforeClose registrations.
| async triggerSnapshotWillSerialize(): Promise<void> { | ||
| debug('trigger snapshotWillSerialize start'); | ||
| const boots = [...this.#boots].reverse(); | ||
| for (const boot of boots) { | ||
| if (typeof boot.snapshotWillSerialize !== 'function') { | ||
| continue; | ||
| } | ||
| const fullPath = boot.fullPath ?? 'unknown'; | ||
| debug('trigger snapshotWillSerialize at %o', fullPath); | ||
| const timingKey = `Snapshot Will Serialize in ${utils.getResolvedFilename(fullPath, this.app.baseDir)}`; | ||
| this.timing.start(timingKey); | ||
| try { | ||
| await utils.callFn(boot.snapshotWillSerialize.bind(boot)); | ||
| } catch (err) { | ||
| debug('trigger snapshotWillSerialize error at %o, error: %s', fullPath, err); | ||
| this.emit('error', err); | ||
| } | ||
| this.timing.end(timingKey); |
There was a problem hiding this comment.
Reject snapshot hook failures instead of only emitting them.
Both snapshot paths emit the error and keep going. That lets snapshot build/restore continue with half-torn-down or half-restored process state, even though these hooks are the only place non-serializable resources get cleaned up and recreated. Please propagate the first failure after emitting it so callers can abort the snapshot operation.
Also applies to: 433-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/lifecycle.ts` around lines 404 - 421, The snapshot hook
error handlers currently only emit errors and continue, which lets snapshot
operations proceed in an inconsistent state; update the catch blocks in
triggerSnapshotWillSerialize and triggerSnapshotDidDeserialize to rethrow the
caught error (after emitting it) so the Promise rejects and callers can abort;
specifically, inside the try/catch around await
utils.callFn(boot.snapshotWillSerialize.bind(boot)) and the analogous call to
boot.snapshotDidDeserialize, call this.emit('error', err) and then throw err
(ensuring timing.end still runs as needed or wrap timing.end in finally if
required) so the first hook failure propagates to the caller.
| this.loadFinished = new Promise<void>((resolve, reject) => { | ||
| this.#loadFinishedResolve = resolve; | ||
| this.#loadFinishedReject = reject; | ||
| }); |
There was a problem hiding this comment.
loadFinished still lets snapshot builds run the normal ready-side effects.
This path now makes snapshot callers wait for load(), but load() still unconditionally registers the this.ready(() => { dumpConfig(); dumpTiming(); dumpManifest(); ... }) callback and #setupTimeoutTimer() in the same file (Lines 246-257 and 634-654). Because snapshot mode marks the lifecycle ready right after configWillLoad, that work can run in the builder process instead of after restore, so snapshot builds still produce normal startup side effects.
Also applies to: 220-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/egg/src/lib/egg.ts` around lines 166 - 169, The loadFinished promise
currently only delays callers but does not prevent load() from registering
ready-side effects; modify load() so that it does not register the ready
callbacks that perform startup side effects (the ready(() => { dumpConfig();
dumpTiming(); dumpManifest(); ... }) registrations) and does not call
`#setupTimeoutTimer`() when running in snapshot/build mode; detect snapshot mode
via the existing snapshot flag/option used by the lifecycle (the same condition
that marks ready after configWillLoad) and skip registering those callbacks and
timers so snapshot builds won't execute dumpConfig/dumpTiming/dumpManifest or
`#setupTimeoutTimer`() in the builder process.
| it('should resolve after load() completes', async () => { | ||
| const app = new Application({ | ||
| baseDir: demoApp, | ||
| mode: 'single', | ||
| snapshot: true, | ||
| }); | ||
| // In snapshot mode, ready() resolves before load() finishes | ||
| await app.ready(); | ||
|
|
||
| // loadFinished should resolve when load() fully completes | ||
| await app.loadFinished; |
There was a problem hiding this comment.
This test doesn't verify the ordering it describes.
await app.ready(); await app.loadFinished; still passes if loadFinished resolves first or both promises settle together. If the contract here is that snapshot ready() resolves before loadFinished, capture that state before awaiting ready() and assert it is still unresolved afterward.
✅ Tighten the assertion
it('should resolve after load() completes', async () => {
const app = new Application({
baseDir: demoApp,
mode: 'single',
snapshot: true,
});
+ let loadFinishedResolved = false;
+ void app.loadFinished.then(() => {
+ loadFinishedResolved = true;
+ });
+
// In snapshot mode, ready() resolves before load() finishes
await app.ready();
+ assert.equal(loadFinishedResolved, false, 'ready() should resolve before loadFinished');
// loadFinished should resolve when load() fully completes
await app.loadFinished;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should resolve after load() completes', async () => { | |
| const app = new Application({ | |
| baseDir: demoApp, | |
| mode: 'single', | |
| snapshot: true, | |
| }); | |
| // In snapshot mode, ready() resolves before load() finishes | |
| await app.ready(); | |
| // loadFinished should resolve when load() fully completes | |
| await app.loadFinished; | |
| it('should resolve after load() completes', async () => { | |
| const app = new Application({ | |
| baseDir: demoApp, | |
| mode: 'single', | |
| snapshot: true, | |
| }); | |
| let loadFinishedResolved = false; | |
| void app.loadFinished.then(() => { | |
| loadFinishedResolved = true; | |
| }); | |
| // In snapshot mode, ready() resolves before load() finishes | |
| await app.ready(); | |
| assert.equal(loadFinishedResolved, false, 'ready() should resolve before loadFinished'); | |
| // loadFinished should resolve when load() fully completes | |
| await app.loadFinished; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/egg/test/snapshot.test.ts` around lines 180 - 190, The test must
assert loadFinished is still pending when ready() resolves: capture the settled
state of app.loadFinished before awaiting ready() (use a small
isPending/isSettled helper or Promise.race trick against a resolved sentinel to
detect whether app.loadFinished has settled), assert it is pending, then await
app.ready(), then assert app.loadFinished remains pending (i.e. not settled)
after ready() resolves, and finally await app.loadFinished to complete the test;
reference Application, app.ready(), and app.loadFinished when locating where to
add the checks.
| // Use lifecycle hooks to clean up non-serializable resources (file handles, | ||
| // timers, process listeners) before snapshot and restore them after deserialize. | ||
| // The hooks are registered internally by Agent and EggApplicationCore constructors. | ||
| if (app.agent) { | ||
| await app.agent.triggerSnapshotWillSerialize(); | ||
| } | ||
| await app.triggerSnapshotWillSerialize(); | ||
|
|
||
| v8.startupSnapshot.setDeserializeMainFunction( | ||
| (snapshotData: SnapshotData) => { | ||
| // This function runs when restoring from snapshot. | ||
| // The application object is available via snapshotData. | ||
| // Users should call restoreSnapshot() to get it. | ||
| globalThis.__egg_snapshot_app = snapshotData.app; | ||
| }, | ||
| { app } as SnapshotData, | ||
| ); |
There was a problem hiding this comment.
buildSnapshot() only registers setDeserializeMainFunction() to stash the app on globalThis, but nothing triggers app.triggerSnapshotDidDeserialize() / agent.triggerSnapshotDidDeserialize() during snapshot restore. Since snapshotWillSerialize() closes the messenger and removes the unhandledRejection handler, a restored process will keep those resources in a "closed/removed" state unless the user manually calls deserialize hooks. Consider registering a V8 deserialize callback (or invoking triggerSnapshotDidDeserialize() from the deserialize main function) and/or adjusting restoreSnapshot() to return an app that has already run the deserialize lifecycle.
| */ | ||
| readonly loadFinished: Promise<void>; | ||
|
|
||
| messenger: IMessenger; |
There was a problem hiding this comment.
This changes messenger from readonly to mutable (messenger: IMessenger) to allow re-creation after snapshot restore. This is a potentially breaking public API/typing change for downstream frameworks that extend EggApplicationCore and rely on readonly messenger. A less breaking approach is to keep the public field readonly (e.g., private #messenger + get messenger()), while updating the private field during snapshotDidDeserialize().
| messenger: IMessenger; | |
| readonly messenger: IMessenger; |
packages/egg/src/lib/egg.ts
Outdated
| // Register snapshot lifecycle hooks for non-serializable resources: | ||
| // - Messenger: holds process listeners (IPC) that cannot survive serialization | ||
| // - Loggers: hold file descriptors | ||
| // - unhandledRejection handler: process-level listener | ||
| // All are cleaned up during serialize and restored during deserialize, | ||
| // avoiding scattered `if (snapshot)` guards throughout the codebase. | ||
| const app = this; |
There was a problem hiding this comment.
The snapshot boot hook aims to avoid snapshot-unsafe resources, but load() still schedules config/timing/manifest dumping, sets the startup timeout timer, and registers the process-level unhandledRejection handler unconditionally (no options.snapshot guard in this file). That contradicts the snapshot-mode contract (“No timers / file writes / listeners”) and risks snapshot build failures or unexpected disk writes. Consider skipping those runtime-only side effects when options.snapshot === true and re-initializing them in snapshotDidDeserialize() instead.
| async triggerSnapshotWillSerialize(): Promise<void> { | ||
| debug('trigger snapshotWillSerialize start'); | ||
| const boots = [...this.#boots].reverse(); | ||
| for (const boot of boots) { | ||
| if (typeof boot.snapshotWillSerialize !== 'function') { | ||
| continue; | ||
| } | ||
| const fullPath = boot.fullPath ?? 'unknown'; | ||
| debug('trigger snapshotWillSerialize at %o', fullPath); | ||
| const timingKey = `Snapshot Will Serialize in ${utils.getResolvedFilename(fullPath, this.app.baseDir)}`; | ||
| this.timing.start(timingKey); | ||
| try { | ||
| await utils.callFn(boot.snapshotWillSerialize.bind(boot)); | ||
| } catch (err) { | ||
| debug('trigger snapshotWillSerialize error at %o, error: %s', fullPath, err); | ||
| this.emit('error', err); | ||
| } | ||
| this.timing.end(timingKey); | ||
| } | ||
| debug('trigger snapshotWillSerialize end'); |
There was a problem hiding this comment.
triggerSnapshotWillSerialize() catches hook errors and only emits 'error', but still continues and ultimately resolves. For snapshot building, cleanup failures (e.g., failing to close a handle) should likely fail the build to avoid producing a snapshot with non-serializable resources. Consider collecting the first error and throwing/rejecting at the end (or rethrowing immediately after emit).
| async triggerSnapshotDidDeserialize(): Promise<void> { | ||
| debug('trigger snapshotDidDeserialize start'); | ||
| for (const boot of this.#boots) { | ||
| if (typeof boot.snapshotDidDeserialize !== 'function') { | ||
| continue; | ||
| } | ||
| const fullPath = boot.fullPath ?? 'unknown'; | ||
| debug('trigger snapshotDidDeserialize at %o', fullPath); | ||
| const timingKey = `Snapshot Did Deserialize in ${utils.getResolvedFilename(fullPath, this.app.baseDir)}`; | ||
| this.timing.start(timingKey); | ||
| try { | ||
| await utils.callFn(boot.snapshotDidDeserialize.bind(boot)); | ||
| } catch (err) { | ||
| debug('trigger snapshotDidDeserialize error at %o, error: %s', fullPath, err); | ||
| this.emit('error', err); | ||
| } | ||
| this.timing.end(timingKey); | ||
| } | ||
| debug('trigger snapshotDidDeserialize end'); | ||
|
|
||
| // Reset ready state for the resumed lifecycle. | ||
| // In snapshot mode, ready(true) was called during triggerConfigWillLoad, | ||
| // resolving the ready promise early. We need fresh ready objects so the | ||
| // resumed lifecycle (didLoad → willReady → didReady) can track properly. | ||
| // Note: keep options.snapshot = true so the constructor's stale ready | ||
| // callback (which may fire asynchronously) correctly skips triggerDidReady. | ||
| this.#readyObject = new ReadyObject(); | ||
| this.#initReady(); | ||
| this.ready((err) => { | ||
| void this.triggerDidReady(err); | ||
| debug('app ready after snapshot deserialize'); | ||
| }); | ||
|
|
||
| // Resume the normal lifecycle from configDidLoad | ||
| this.triggerConfigDidLoad(); | ||
|
|
||
| // Wait for the full resumed lifecycle to complete | ||
| await this.ready(); | ||
| } |
There was a problem hiding this comment.
triggerSnapshotDidDeserialize() similarly swallows hook errors by emitting 'error' and continuing, then proceeds to resume the lifecycle. If any deserialize hook fails, continuing may leave the process partially restored (e.g., missing listeners/timers) but still appear “ready”. Consider propagating errors (throw/reject) so callers can abort startup on restore failures.
packages/egg/src/lib/start.ts
Outdated
| * Creates the application with `snapshot: true`, which causes the lifecycle | ||
| * to stop after `configWillLoad`. The loader still runs fully (extensions, | ||
| * services, controllers) via `loadFinished`. No servers, timers, file | ||
| * watchers, or connections are created. |
There was a problem hiding this comment.
The docstring promises snapshot mode creates “No servers, timers, file watchers, or connections”, but EggApplicationCore.load() currently sets up a startup timeout timer and schedules config/timing/manifest dumping even when snapshot: true (no guard). Either add the necessary snapshot guards so this statement is true, or soften the doc to match the current behavior.
| * Creates the application with `snapshot: true`, which causes the lifecycle | |
| * to stop after `configWillLoad`. The loader still runs fully (extensions, | |
| * services, controllers) via `loadFinished`. No servers, timers, file | |
| * watchers, or connections are created. | |
| * Creates the agent and application with `snapshot: true`, which causes the | |
| * lifecycle to stop after `configWillLoad`. The loader still runs fully | |
| * (extensions, services, controllers) via `loadFinished`, ensuring all | |
| * framework metadata is available for snapshotting. |
killagu
force-pushed
the
feat/snapshot-egg
branch
from
e16fa39 to
3760efd
Compare
killagu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/egg/src/lib/start.ts (1)
98-138: Well-structured snapshot startup with correct lifecycle handling.The implementation correctly:
- Uses
snapshot: trueto stop lifecycle afterconfigWillLoad- Awaits
loadFinishedinstead ofready()to ensure full loading- Creates both agent and application (unlike
metadataOnlyinstartEgg)- Omits
egg-readybroadcast (noserverDidReadyduring snapshot build)Consider extracting the duplicated framework resolution logic (lines 101-117) shared with
startEgg(lines 44-60) into a helper function to reduce code duplication.♻️ Optional: Extract shared framework resolution
async function resolveFrameworkClasses( baseDir: string, framework?: string, ): Promise<{ AgentClass: typeof Agent; ApplicationClass: typeof Application }> { let AgentClass = Agent; let ApplicationClass = Application; if (framework) { const mod = await importModule(framework, { paths: [baseDir] }); AgentClass = mod.Agent; ApplicationClass = mod.Application; } return { AgentClass, ApplicationClass }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/src/lib/start.ts` around lines 98 - 138, The framework resolution logic duplicated between startEggForSnapshot and startEgg should be extracted into a helper (e.g., resolveFrameworkClasses) that accepts baseDir and optional framework and returns { AgentClass, ApplicationClass }; replace the inline blocks in startEggForSnapshot (the code that sets AgentClass/ApplicationClass and calls importModule) and the corresponding block in startEgg with calls to that helper to eliminate duplication and keep existing behavior (default to Agent/Application if no framework, importModule with paths: [baseDir] when framework provided).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/egg/src/lib/snapshot.ts`:
- Around line 29-49: Update restoreSnapshot to be async and invoke the
deserialize lifecycle hook so non-serializable resources are restored: change
restoreSnapshot to return Promise<App> (make it async), retrieve the app from
globalThis.__egg_snapshot_app, then call and await
app.triggerSnapshotDidDeserialize() before returning the app; also update or
remove the existing JSDoc line that claims loggers/messenger are automatically
re-created if you prefer to keep documentation accurate. Ensure references to
restoreSnapshot and triggerSnapshotDidDeserialize (and
globalThis.__egg_snapshot_app) are adjusted accordingly.
---
Nitpick comments:
In `@packages/egg/src/lib/start.ts`:
- Around line 98-138: The framework resolution logic duplicated between
startEggForSnapshot and startEgg should be extracted into a helper (e.g.,
resolveFrameworkClasses) that accepts baseDir and optional framework and returns
{ AgentClass, ApplicationClass }; replace the inline blocks in
startEggForSnapshot (the code that sets AgentClass/ApplicationClass and calls
importModule) and the corresponding block in startEgg with calls to that helper
to eliminate duplication and keep existing behavior (default to
Agent/Application if no framework, importModule with paths: [baseDir] when
framework provided).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82c42ac7-c7c7-49ad-b9e6-c70431d98e81
📒 Files selected for processing (9)
packages/core/src/egg.tspackages/core/src/lifecycle.tspackages/core/test/snapshot.test.tspackages/egg/src/index.tspackages/egg/src/lib/agent.tspackages/egg/src/lib/egg.tspackages/egg/src/lib/snapshot.tspackages/egg/src/lib/start.tspackages/egg/test/snapshot.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/egg/src/index.ts
- packages/egg/test/snapshot.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/egg.ts
- packages/core/src/lifecycle.ts
- packages/core/test/snapshot.test.ts
packages/egg/src/lib/snapshot.ts
Outdated
| export async function buildSnapshot(options: SnapshotEggOptions = {}): Promise<void> { | ||
| const app = await startEggForSnapshot(options); | ||
|
|
||
| // Use lifecycle hooks to clean up non-serializable resources (file handles, | ||
| // timers, process listeners) before snapshot and restore them after deserialize. | ||
| // The hooks are registered internally by Agent and EggApplicationCore constructors. | ||
| if (app.agent) { | ||
| await app.agent.triggerSnapshotWillSerialize(); | ||
| } | ||
| await app.triggerSnapshotWillSerialize(); | ||
|
|
||
| v8.startupSnapshot.setDeserializeMainFunction( | ||
| (snapshotData: SnapshotData) => { | ||
| // This function runs when restoring from snapshot. | ||
| // The application object is available via snapshotData. | ||
| // Users should call restoreSnapshot() to get it. | ||
| globalThis.__egg_snapshot_app = snapshotData.app; | ||
| }, | ||
| { app } as SnapshotData, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Node.js v8.startupSnapshot.setDeserializeMainFunction support async callback functions?
💡 Result:
No, Node.js v8.startupSnapshot.setDeserializeMainFunction does not support async callback functions. The callback must be a synchronous function of type StartupSnapshotCallbackFn, which takes optional data as argument and returns void. All official documentation and examples show synchronous arrow functions or function expressions that perform synchronous operations like console.log. Asynchronous operations, such as those using await or returning promises, are not supported because the callback is invoked directly as the application entry point after deserialization, and Node.js expects the process to run synchronously without awaiting. A GitHub discussion demonstrates an async IIFE inside the callback, but the callback itself remains synchronous; the async code runs but does not block or await the process exit.
Citations:
- 1: https://bun.com/reference/node/v8/startupSnapshot/setDeserializeMainFunction
- 2: https://bun.com/reference/node/v8/startupSnapshot
- 3: https://docs.deno.com/api/node/v8/~/startupSnapshot
- 4: Use v8 snapshots nodejs/single-executable#57
- 5: https://nodejs.org/api/v8.html
- 6: v8: add v8.startupSnapshot utils nodejs/node#43329
- 7: http://www.nodejs.org/api/v8.html
🏁 Script executed:
# Search for restoreSnapshot implementation
rg "restoreSnapshot" -A 10 packages/egg/src/lib/snapshot.tsRepository: eggjs/egg
Length of output: 1219
🏁 Script executed:
# Search for snapshotDidDeserialize calls in the codebase
rg "snapshotDidDeserialize" packages/ -A 2 -B 2Repository: eggjs/egg
Length of output: 10484
🏁 Script executed:
# Check how snapshot is used in tests or examples
fd -e ts -e js | xargs rg "buildSnapshot|restoreSnapshot" --max-count=3 packages/Repository: eggjs/egg
Length of output: 2208
🏁 Script executed:
# Search for where triggerSnapshotDidDeserialize is actually called
rg "triggerSnapshotDidDeserialize" --type ts -B 3 -A 3 packages/Repository: eggjs/egg
Length of output: 7539
🏁 Script executed:
# Look for snapshot restore entry points or initialization logic
rg "setDeserializeMainFunction" -B 5 -A 15 packages/Repository: eggjs/egg
Length of output: 1435
🏁 Script executed:
# Check the Application/EggApplicationCore to see if restoreSnapshot should trigger lifecycle
rg "class Application|class EggApplicationCore" -A 30 packages/egg/src/lib/Repository: eggjs/egg
Length of output: 3942
🏁 Script executed:
# Check the full snapshot.ts file to see if there's any guidance on the restore workflow
cat -n packages/egg/src/lib/snapshot.ts | head -80Repository: eggjs/egg
Length of output: 3228
🏁 Script executed:
# Look for snapshot test setups to understand the expected restore pattern
rg "buildSnapshot|restoreSnapshot|triggerSnapshotDidDeserialize" packages/egg/test/snapshot.test.ts -B 2 -A 2 | head -100Repository: eggjs/egg
Length of output: 1247
🏁 Script executed:
# Check if there's a restore entry point or documentation
rg "restore.*entry|entry.*restore" packages/ -lRepository: eggjs/egg
Length of output: 108
Fix misleading JSDoc—deserialize lifecycle hooks are not automatically triggered.
The restoreSnapshot() JSDoc (lines 54-57) claims "Loggers and messenger have been automatically re-created by the deserialize callbacks," but the deserialize callback only stores the app to globalThis without triggering the lifecycle hooks. Users following this documented pattern would receive an app with closed messenger and stale loggers.
Either:
- Have
restoreSnapshot()callawait app.triggerSnapshotDidDeserialize()internally (requires making it async), or - Update the JSDoc to clarify that users must manually call
app.triggerSnapshotDidDeserialize()afterrestoreSnapshot()to restore non-serializable resources.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/egg/src/lib/snapshot.ts` around lines 29 - 49, Update
restoreSnapshot to be async and invoke the deserialize lifecycle hook so
non-serializable resources are restored: change restoreSnapshot to return
Promise<App> (make it async), retrieve the app from
globalThis.__egg_snapshot_app, then call and await
app.triggerSnapshotDidDeserialize() before returning the app; also update or
remove the existing JSDoc line that claims loggers/messenger are automatically
re-created if you prefer to keep documentation accurate. Ensure references to
restoreSnapshot and triggerSnapshotDidDeserialize (and
globalThis.__egg_snapshot_app) are adjusted accordingly.
8cd334e to
5d90728
Compare
| (snapshotData: SnapshotData) => { | ||
| // This function runs when restoring from snapshot. | ||
| // The application object is available via snapshotData. | ||
| // Users should call restoreSnapshot() to get it. | ||
| globalThis.__egg_snapshot_app = snapshotData.app; |
There was a problem hiding this comment.
buildSnapshot() closes/cleans resources via triggerSnapshotWillSerialize(), but the V8 setDeserializeMainFunction callback only stores snapshotData.app on globalThis and never triggers snapshotDidDeserialize hooks. After restoring from the snapshot the app/agent will still have messenger closed, loggers cleared, and process listeners removed. Consider invoking (and ideally awaiting) triggerSnapshotDidDeserialize() for both snapshotData.app and snapshotData.app.agent inside the deserialize main function (or capture a restore promise that restoreSnapshot() can await).
| (snapshotData: SnapshotData) => { | |
| // This function runs when restoring from snapshot. | |
| // The application object is available via snapshotData. | |
| // Users should call restoreSnapshot() to get it. | |
| globalThis.__egg_snapshot_app = snapshotData.app; | |
| async (snapshotData: SnapshotData) => { | |
| // This function runs when restoring from snapshot. | |
| // The application object is available via snapshotData. | |
| // Users should call restoreSnapshot() to get it. | |
| const app = snapshotData.app; | |
| globalThis.__egg_snapshot_app = app; | |
| // Recreate resources that were cleaned up during snapshot serialization. | |
| if (app.agent) { | |
| await app.agent.triggerSnapshotDidDeserialize(); | |
| } | |
| await app.triggerSnapshotDidDeserialize(); |
| /** | ||
| * Restore an egg application from a V8 startup snapshot. | ||
| * | ||
| * Returns the Application instance that was captured during snapshot | ||
| * construction. The application has all metadata pre-loaded (plugins, | ||
| * configs, extensions, services, controllers, router). Loggers and | ||
| * messenger have been automatically re-created by the deserialize callbacks. | ||
| */ | ||
| export function restoreSnapshot(): Application { | ||
| const app = globalThis.__egg_snapshot_app; | ||
| if (!app) { | ||
| throw new Error( | ||
| 'No egg application found in snapshot. ' + | ||
| 'Ensure the process was started from a snapshot built with buildSnapshot().', | ||
| ); | ||
| } | ||
| return app as Application; | ||
| } |
There was a problem hiding this comment.
The restoreSnapshot() docstring says messenger/loggers “have been automatically re-created by the deserialize callbacks”, but restoreSnapshot() only returns the global app reference and does not ensure any deserialize lifecycle has run/completed. Either run the deserialize hooks as part of restoration (potentially making restoreSnapshot() async) or adjust the documentation/API so callers can reliably wait for restoration before using the app.
killagu
force-pushed
the
feat/snapshot-egg
branch
from
d507547 to
e044a5d
Compare
killagu
force-pushed
the
feat/snapshot-egg
branch
from
3a16dce to
49a0781
Compare
| readonly loadFinished: Promise<void>; | ||
|
|
||
| messenger: IMessenger; | ||
| agent?: Agent; | ||
| application?: Application; |
There was a problem hiding this comment.
messenger was changed from readonly to mutable so it can be replaced after snapshot deserialize. This is a TypeScript-visible API breaking change (consumers can now reassign, and existing code that relied on readonly will break). To preserve API contracts, consider making messenger a private field with a read-only getter, and only mutate internally during deserialize.
| @@ -73,11 +107,25 @@ export async function startEgg(options: StartEggOptions = {}): Promise<SingleMod | |||
| application.agent = agent; | |||
| agent.application = application; | |||
There was a problem hiding this comment.
SingleModeAgent declares an app: SingleModeApplication property, but startEgg() only links the instances via agent.application = application. Since Agent doesn’t define an app property, this interface is misleading and callers may observe agent.app as undefined at runtime. Either assign agent.app = application (if that’s the intended public alias) or update the type to reflect the actual application property.
| agent.application = application; | |
| agent.application = application; | |
| agent.app = application; |
Add snapshotWillSerialize/snapshotDidDeserialize lifecycle hooks and
snapshot-safe initialization patterns for V8 startup snapshot support.
## core changes
- Add `snapshotWillSerialize()` / `snapshotDidDeserialize()` to ILifecycleBoot
- triggerSnapshotWillSerialize: reverse order (LIFO, like beforeClose)
- triggerSnapshotDidDeserialize: forward order (FIFO), resumes lifecycle from configDidLoad
- Unified ready() behavior: snapshot mode waits for load completion naturally
- Expose trigger methods on EggCore for build scripts
## egg changes
- Agent keepalive timer moved to configDidLoad via boot hook
- Messenger/logger/unhandledRejection cleanup via protected lifecycle methods
- Zero `if (this.options.snapshot)` guards — all handled by lifecycle structure
- `startEgg({ snapshot: true })` for snapshot mode (unified API)
- Shared resolveFrameworkClasses() helper eliminates duplication
- buildSnapshot() / restoreSnapshot() public APIs
## Tests
- 16 core snapshot tests (hook ordering, async, error handling, lifecycle resume)
- 11 egg snapshot tests (agent keepalive, messenger, logger, unhandledRejection, APIs)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
killagu
force-pushed
the
feat/snapshot-egg
branch
from
aa0a125 to
6c29b67
Compare
jerryliang64
left a comment
jerryliang64
left a comment
There was a problem hiding this comment.
Minor review comments on three low-severity items.
| }); | ||
| this._unhandledRejectionHandler = this._unhandledRejectionHandler.bind(this); | ||
| process.on('unhandledRejection', this._unhandledRejectionHandler); | ||
| } |
There was a problem hiding this comment.
_unhandledRejectionHandler 在 load() 方法中已经 bind 过一次了（egg.ts:204）：
this._unhandledRejectionHandler = this._unhandledRejectionHandler.bind(this);快照恢复后，对象上的 _unhandledRejectionHandler 已经是 bound 版本，这里再 bind 一次会产生 bound bound _unhandledRejectionHandler。功能上没影响，但属于冗余操作。
建议去掉这行 bind，直接 process.on('unhandledRejection', this._unhandledRejectionHandler) 即可。
| * Load egg application metadata for V8 startup snapshot construction. | ||
| * | ||
| * @deprecated Use `startEgg({ ...options, snapshot: true })` instead. | ||
| */ |
There was a problem hiding this comment.
startEggForSnapshot 从未发布过就标记 @deprecated，属于 born-deprecated API。如果是为了和 umbrella PR (#5849) 中的引用过渡兼容，建议在这里加个注释说明原因；否则建议直接删除，避免发布一个一出生就废弃的公共 API。
| */ | ||
| startKeepAlive(): void { | ||
| if (this.#agentAliveHandler) return; | ||
| this.#agentAliveHandler = setInterval( |
There was a problem hiding this comment.
startKeepAlive() 原本是构造函数里的内部逻辑，现在变成了 public method。但它只被内部 configDidLoad boot hook 通过箭头函数调用，外部不应该直接调用它。
变成 public 似乎是为了测试中直接调用 agent.startKeepAlive() 验证幂等性。建议改为 protected，测试可以通过子类或 (agent as any).startKeepAlive() 访问。
Summary
Add V8 startup snapshot support to the egg framework package:
snapshot: trueoption triggers lifecycle cutoff afterconfigWillLoad(via PR2 core), then awaitsloadFinishedfor loader completionbuildSnapshot()/restoreSnapshot()APIs: Public entry points for snapshot construction and restorationstartEggForSnapshot(): Creates Agent + Application in single/snapshot mode, awaits full loader completion vialoadFinishedpromiseregisterSnapshotCallbacks(): Serialize/deserialize hooks for loggers, messenger, agent keep-alive timer, unhandledRejection handler, HttpClient prototypesnapshot: trueThis is PR4 of 6 in the V8 startup snapshot series. Depends on PR1 (koa), PR2 (core), PR3 (utils).
Changes (7 files, +294/-47)
packages/egg/src/lib/egg.ts— Snapshot option, loadFinished promise, registerSnapshotCallbacks(), guardspackages/egg/src/lib/agent.ts— Conditional keepalive, registerSnapshotCallbacks() overridepackages/egg/src/lib/application.ts— Skip bindEvents in snapshot modepackages/egg/src/lib/snapshot.ts— NEW: buildSnapshot/restoreSnapshot APIspackages/egg/src/lib/start.ts— NEW: startEggForSnapshot()packages/egg/src/index.ts— Export snapshot utilitiespackages/egg/test/lib/snapshot.test.ts— NEW: 18 comprehensive testsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests